Skip to content

[IDE] Title of sketch is misaligned on Linux FIX #10209 #10210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 20, 2020

Conversation

ricardojlrufino
Copy link
Contributor

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Details

For a long time I always found the linux icon out of alignment, I attribute this problem that was perhaps related to my system fonts. But I think that need a HACK for Linux / Ubuntu

Tests:
Os: Kubuntu / KDE
JDK: OpenJDK 7 and 11

But I decided to investigate further, and looking at some prints on the internet I see that it is really out of alignment.

http://dexvils.blogspot.com/2015/06/install-arduino-ide-16x-on-ubuntu.html
https://websiteforstudents.com/how-to-install-arduino-ide-on-ubuntu-18-04-16-04/

image

@matthijskooijman
Copy link
Collaborator

Thanks for the patch. However:

  • Are you sure this is a Linux-only issue?
  • I'm not quite inclined to make a OS-specific hardcoded 2-pixel exception here, without knowing for certain what causes the problem. If the problem is font-specific rather than OS-specific, the fix might actually unalign things with other fonts maybe.
  • Are you sure that it is actually misaligned? Looking at the code, it tries to center the "ascent" of the font: "The font ascent is the distance from the font's baseline to the top of most alphanumeric characters. Some characters in the Font might extend above the font ascent line.". I suspect the ascent might be the size from e.g. bottom and top of letters like the "s". In your screenshot, the "s" actually looks nicely centered. It's just that some letters stick out above the ascent line (but others will stick out below the baseline). So this might actually work as designed.
  • if we indeed want to center the entire character, rather than just the smaller ones, it might be better to use getMaxAscent() instead (which returns the top of the biggest char, I believe). In that case, it might also make sense to account for the (max) descent as well. No clue if this will actually look much better.
  • All in all, I wonder if this is really worth spending time on, though.

@cmaglie
Copy link
Member

cmaglie commented May 15, 2020

Before the patch:
image

image

that looks off 2 pixels up.
After the patch:

image

image

but now it looks off 2 pxels down. IMHO on my system the correct offset should be +1 pixel and not +2. Maybe we should not look the ascent as suggested by @matthijskooijman.

@ricardojlrufino
Copy link
Contributor Author

ricardojlrufino commented May 15, 2020

I had a test using Descent to calculate "new base line" e looks good.

old = int baseline = (sizeH + fontAscent) / 2;
new = int baseline = (sizeH + fontAscent + metrics.getDescent()) / 2;

The questios:

Are you sure this is a Linux-only issue?

On linux it seems to be more evident, but maybe it's the fact that it's my default OS. The modifications I made tested only on Windows and Linux, and according to the images below it seems to me great.

Linux

image

Windows

image

All in all, I wonder if this is really worth spending time on, though.

I believe that it really isn't one of the most interesting pull requests I've ever made ... =]
But I always felt uncomfortable with this alignment.
I think it's worth testing and seeing if it looks good on other systems:
I attached the JARs to replace in the lib folder, and do a quick check.

arduino-fix-title.zip

@ricardojlrufino
Copy link
Contributor Author

@matthijskooijman
Copy link
Collaborator

I'm not sure if this is really a correct approach though, or whether it just happens to look good.

Remember that you're calculating the baseline position, since the baseline will be drawn in the position calculated.

new = int baseline = (sizeH + fontAscent + metrics.getDescent()) / 2;

I think this happens to work because the Descent is just 2 pixels. Consider what would happen with a (admittedly weird) font where the descent is 10 pixels. This would end up shifting the baseline 5 pixels down, and the extra descent will extend even below that baseline.

Looking more closely at @cmaglie's screenshot, which shows the individual pixels a bit more clearly, I'm a bit surprised. I previously thought that the centering might be correct, but just only the bases of the letters (so excluding e.g. the pole of the b) and the poles just make it look incorrectly centered. But @cmaglie shows that it is actually not doing that, the bit that is centered (e.g. the area that is enclosed between the baseline and a imaginary line above the center at the same distance as the baseline is under the center) is actually smaller than the b-without-pole-area.

To make things even more confusing, the image linked by @ricardojlrufino shows that the ascent is actually longer than I had interpreted it (it includes the pole of the b or d). So if getAscent() returns a higher value than I gathered, I think the text should have ended up lower not higher than I expected.

So, I actually suspect that something else might be going on:

  • Maybe sizeH is not correct
  • Maybe the position does not begin at the top of the "tab" but there is a bit of a border?
  • Maybe the baseline (the part of the text that gets put at the given coordinates) is not what we think it is?
  • Maybe some rounding errors too?

It might be useful to print the values of sizeH, getAscent() and baseline, and then make a screenshot (probably with original code) and count to see if these things are correct. Maybe also draw a single-column rectangle just before the ext from row 0 to row baseline to doublecheck where these pixels are exactly?

@ricardojlrufino
Copy link
Contributor Author

Place a few lines of reference:

And looking for original code: int baseline = (sizeH + fontAscent) / 2;
It became clearer that the text is being drawn in the middle of the Header not the Tab
also has the rounding problem, because the height size is 35 (so let's have a pixel less ..)

    int baseline = (sizeH + fontAscent) / 2;
    
    g.setColor(Color.GREEN);
    g.drawRect(0, 0, size.width - 1, size.height - 1);
    g.drawLine(0, size.height/2, size.width, size.height / 2);
    g.drawString("baseline test: GgyYiI", 300, size.height/2);
    // test text  bounds
    g.setColor(Color.RED);
    g.drawRect(0, baseline - fontAscent, 20, fontAscent);

image

It seems that the problem is in the fontAscent which has a space left over
getMaxAscent and getAscent, has same value = 12

image

Output:

size.width = 492 sizeW = | 492
size.height = 35 sizeH | 35
metrics.getAscent = 12 getDescent | 3
baseline=23
middle=17

@ricardojlrufino
Copy link
Contributor Author

ricardojlrufino commented May 15, 2020

These pixels more than fontAscent are probably for accented capital letters like: Ó, which should not exist in our case.

Coincidentally this problem caused the text to go down, and "look" centered on the tab

@ricardojlrufino
Copy link
Contributor Author

Then we have: 3 pixels of the font, which are not visible .... and 1 pixel lost in the rounding of the header size (35)

Dividing by 2, we have the 2 pixels that I had initially used to correct ...

@matthijskooijman
Copy link
Collaborator

Nice, good to have a detailed idea of what is going on.

So, there's two problems:

  1. It is centering on the bar, not the tab.
  2. The ascent returned is not what we expected it to be.

Since both introduce opposite errors, most of the error is cancelled out.

Fixing 1. is obvious, just adjust the calculation to use the actual tab size. This is probably easiest by introducing a constant into the code that hardcodes the tab size (since I think the tab images are full-height with some transparent pixels at the top, so you cannot easily derive it from those).

Fixing 2. is more tricky, since we rely on java and/or the font to return meaningful values. It really looks like the returned value is wrong, though, I wonder if there are any characters at all that extend up to the full ascent value?

We can hardcode an offset for this particular font, but different systems and different fonts might then get messed up again. One approach could be to check if the ascent and max ascent are equal, and if so, apply a correction on the ascent (still not ideal, but at least other fonts that do make a distinction will not get over-corrected then).

@ricardojlrufino
Copy link
Contributor Author

We can hardcode an offset for this particular font, but different systems and different fonts might then get messed up again

  1. The fonts are defined by the theme: header.text.font = SansSerif, plain, 12
  2. Whoever changes the fonts, is susceptible to it not being visually pleasing.
    So the idea of having some constant hardcodes can be feasible

The ascent returned is not what we expected it to be.

java.awt.FontMetrics getAscent() returns getMaxAscent()
Yes, it looks like an old bug..
Since JDK 1.2, for the same TrueType fonts, the value has been retrieved from the 'hhea' table's 'ascent' field.
https://bugs.openjdk.java.net/browse/JDK-6623223

image

I wonder if there are any characters at all that extend up to the full ascent value?

If these characters exist today, the misalignment for them will become more evident in the current implementation.

@ricardojlrufino
Copy link
Contributor Author

What do you think about having the 'hardcoded' ajust for the default font "SansSerif" and highlight it as a fix for this Java bug. That could be removed when the bug is resolved (I doubt it ... lol) ?

This is a problem that has existed for a long time ... based on the screenshots of the IDE's prints that I've been looking for on the internet.

@ricardojlrufino
Copy link
Contributor Author

Using:
image

I get:
image

Some lines for reference
image

The result was that the baseline was slightly lower, but the perception is that it is still aligned. Perhaps because of the rounded edges of the tab, the brain (at least mine) understands that there is a "suitable" place to be

@matthijskooijman
Copy link
Collaborator

Hm, I had considered suggesting this (fixing 1. and leaving 2. broken), but decided that would not look good so I didn't. Turns out it does actually look ok indeed. Apparently slightly too low is ok, while slightly too high is not :-)

The code looks good to me, if you push that to this PR, I'd approve it :-)

Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

Would still need to be squashed into a single commit before merging, though.

@cmaglie
Copy link
Member

cmaglie commented May 20, 2020

Tested on Windows looks fine too. Thanks!

@cmaglie cmaglie merged commit cc65234 into arduino:master May 20, 2020
@cmaglie cmaglie added this to the Release 1.8.13 milestone May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants